-
Notifications
You must be signed in to change notification settings - Fork 646
Unifies server module library and client SDK for TypeScript (and fixes several bugs) #3559
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
cloutiertyler
wants to merge
43
commits into
master
Choose a base branch
from
tyler/ts-sdk-improvements
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+7,188
−15,024
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
Notes from PR review meeting:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of Changes
This PR is a very large change to the workings of the TypeScript SDK and as such requires a higher bar of testing than other PRs. However, it does several important things:
RemoteModuletype which vastly cleans up the correctness of type checking on the client and helped me to find several small bugsIt accomplishes this by changing code generation of TypeScript on the client to code generation approximately what a developer would manually write in their module. The ultimate goal would be to allow the developer to use the types and functions that they define on in their module directly on the client without needing to do any code generation at all, provided they are using TypeScript on the server and client.
#3365 is resolved by
.build()ing theDbConnectioninside a ReactuseEffectrather than doing it directly in line with the render of the provider. In order to do that we needed to not expose theDbConnectiondirectly to developers by returning a different type fromuseSpacetimeDB.useSpacetimeDBnow returns aConnectionStateobject which is stored as React state and updates when any of the fields change. This change also resolves #3431.#3435 was the issue that initially lead me down the rabbit hole of unifying the server and the client because it was nearly impossible to track down all the various type functions and how they connect to the values that we code generate on the server. After several hours of attempting this, I decided to clean up the types a bit to be more uniform.
Implementing the unification between the client and the server also necessitated fully implemented parts of the API that were fully implemented on the server, but were broken or missing on the client.
API and ABI breaking changes
[Unification] -> Means that this is breaking behavior for the client SDK, but that the new behavior is identical to the server's existing behavior
Breaking changes:
Table accessor names and index accessor names are converted to camelCase on the
ctx, soctx.db.foo_baris nowctx.db.fooBar[Unification] On the client
my_table.iter()returnsIterableIteratorinstead of anArray[Unification]
module_bindingsnow exportTypeBuilders for all types instead of atype MyTypeand objectMyType, so instead of usingMyTypeas a type directly, you need to infer the typeMyType->Infer<typeof MyType>.[Unification] We no longer generate and export
MyTypeVariantsfor sum types (these are now accessed byInfer<typeof MyType.variants.myVariant>)[Unification]
MyType.getTypeScriptAlgebraicType()has been replaced withMyType.algebraicTypeuseSpacetimeDB()no longer takes type parametersuseTable()now takes aTableDefparameter and type params are inferreduseTable()now just returns anArraydirectly instead of a object with{ rows }[Unification]
ctx.reducers.createPlayer(argA, argB)->ctx.reducers.createPlayer({ argA, argB })[Unification]
ctx.reducers.onCreatePlayer(ctx, argA, argB)->ctx.reducers.onCreatePlayer(ctx, { argA, argB })[Unification]
ctx.reducers.removeOnCreatePlayer(ctx, argA, argB)->ctx.reducers.removeOnCreatePlayer(ctx, { argA, argB })[Unification]
myTable.count(): number->myTable.count(): bigintAdditive changes:
Infer<>now also doesInferTypeOfRow<>if applicableuseReducer()React hookmodule_bindingsnow exports atablesobject with references to all theTableDefsmodule_bindingsnow exports areducersobject with references to all theReducerDefsMyType.create('MyVariant', ...)function in addition to theMyType.MyVariant(...)constructors (this is private)Notable things that did not change:
MyType.serialize(writer: BinaryWriter, value: Infer<typeof MyType>)andMyType.deserialize(reader: BinaryReader): Infer<typeof MyType>are still supported exactly as before.MyType.MyVariant(...)constructor function on sum types is still present, but implemented with the privateMyType.create('MyVariant', ...). We could choose to move away from this API later if we didn't like the variants polluting the namespaceExpected complexity level and risk
4 - This is a deep reaching an complex change for the SDK. For the server, it is much less deep reaching since it reuses much of the same machinery, although it does require thorough testing there as some of the code was modified.
This change is fully localized to TypeScript and does not touch the host (or other languages) at all, and therefore only impacts a beta aspect of SpacetimeDB.
Testing
test-appandtest-react-router-app